-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Listen on printing shortcut to open print view for printing #544
Conversation
Passing run #983 ↗︎
Details:
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Just one minor fix to prevent overwriting other keyboard shortcuts.
(Have not tried it - just from the sourcecode it seems ctrl+shift+P migth also be affected.)
57d0d68
to
58bd7f6
Compare
58bd7f6
to
4d171cb
Compare
42312e9
to
14541e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @susnux! This would make it impossible to print single pages though, no? Or am I missing something?
0fce9d0
to
ff789f4
Compare
🤦 I did not noticed that you currently print the whole collective with subpages... The idea was to not duplicate places with printing specific styling. So forward to the export view, I am currently thinking about resolving this by adding a property to the print view to only print the current page, do you think this is a acceptable solution? |
Yeah, in the long run I would like to add a dedicated export view that allows to print or export pages to PDF. The default should be to export the whole collective, but it should be possible to (un)select specific pages. Unfortunately this is not very high on the priority list. See #463. Your intermediate solution to allow printing only a specific page in the print view sounds good to me! |
ff789f4
to
bb18d7c
Compare
I am currently try to work on this again, I think the best way to solve this is to add subpage support for the export view as well. But this will require quite some work on that view (as I am not that into the collectives app structure). I hope to get this working this month :) |
bb18d7c
to
1c04476
Compare
It is now possible to use the print view to only print subpages, this is now also used for |
de87d59
to
b60cd65
Compare
9aa720a
to
afbc924
Compare
@mejo- Any idea why only one cypress ci was successfully? |
* Listen on printing shortcut to open print view for printing * Hide toastify dialogs when printing * Make sure background is removed Signed-off-by: Ferdinand Thiessen <[email protected]>
afbc924
to
50d77bc
Compare
Never mind CI is green after rebasing 🎉 |
The images in the initial PR post look like this also solves the problem of the content being moved on to the 2nd page when it's longer than 1 page (no page break). I find that every collective I try to print longer than a specific amount has this issue - the title on the first page and content on the 2nd page. Hopefully this can be merged soon! |
@@ -142,6 +142,7 @@ export default { | |||
'isCollectiveAdmin', | |||
'isPublic', | |||
'loading', | |||
'printLink', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a leftover, printLink()
is defined as computed property below and not available as getter from the store.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave this a quick test. Unfortunately I ran into several issues:
- It doesn't work for first-level pages because they somehow get a double slash in the URL.
- It doesn't work for second-level pages (and first-level pages with the double slash removed). Console error is:
this.currentPage is undefined
.
@@ -166,6 +167,10 @@ export default { | |||
? generateUrl(`/apps/collectives/p/${this.shareTokenParam}/print/${this.collective.name}`) | |||
: generateUrl(`/apps/collectives/_/print/${this.collective.name}`) | |||
}, | |||
|
|||
unshareIcon() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a leftover or copy/paste error as well. unshareIcon()
is not used here.
@@ -171,6 +171,14 @@ export default { | |||
return state.pages.find(p => (p.parentId === parentId && p.title === TEMPLATE_PAGE)) | |||
}, | |||
|
|||
pagePrintLink: (state, get) => (page) => { | |||
const path = [page.collectivePath.split('/').at(-1), page.filePath.split('/'), page.fileName] | |||
if (path.at(-1) === 'Readme.md') path.splice(-1, 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use these one-line if () ...
syntax so far, I'd prefer to stick to if () { \n ... \n }` 😬
Same for the following two lines.
printKeyHandler(event) { | ||
// Handle `CTRL+P` or `CMD+P` but ensure ALT or SHIFT are NOT pressed (e.g. CTRL+SHIFT+P is new private tab on firefox) | ||
if ((event.metaKey || event.ctrlKey) && event.key.toLowerCase() === 'p' && !(event.altKey || event.shiftKey)) { | ||
if (!this.currentPage) return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
@@ -37,11 +37,13 @@ const routes = [ | |||
path: '/_/print/:collective', | |||
component: CollectivePrintView, | |||
props: (route) => route.params, | |||
children: [{ path: ':page*' }], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give me a quick hint what you want to achieve here? Maybe I oversaw something, but I don't see where this would be used 🤔
Also resolves the problem with empty page before first page content on Firefox. Fixes: #544 Fixes: #542 Signed-off-by: Jonas <[email protected]>
Also resolves the problem with empty page before first page content on Firefox. Fixes: #544 Fixes: #542 Signed-off-by: Jonas <[email protected]>
Closing this PR now that we have fixes from #971. |
📝 Summary
Listen on CTRL + P (or CMD respectivly), then cancel printing the collective but open the print view to the correct styling get applied.
Moreover I changed the target name of the new tab / window, so the printing view gets recycled for all printing tasks to not spam the users tabs.
🖼️ Screenshots
🏁 Checklist
npm run lint
/npm run stylelint
/composer run cs:check
)